Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce instances of font fallback dialog #9734

Merged
9 commits merged into from
Apr 8, 2021
Merged

Reduce instances of font fallback dialog #9734

9 commits merged into from
Apr 8, 2021

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Apr 6, 2021

Reduce instances of font fallback dialog through package font loading,
basic name trimming, and revised fallback test

  • Adjusts the font dialog to only show when we attempt last-chance
    resolution from our hardcoded list of font names with a flag instead
    of with a string comparison by name

  • Adds a resolution step to trim the font name by word from the end and
    retry to attempt to resolve a proper font that just has a weight
    suffix

  • Adds a second font collection to font loading that will attempt to
    locate all TTF files sitting next to our binary, like in our package

  • Wrote my font preference in the JSON as Cascadia Code Heavy and
    watched it quietly resolve to just Cascadia Code without the dialog.

  • Put a font that isn't registered with the system into the layout
    directory for the package, set it as my desired font in Terminal, and
    watched it load just fine.

  • Try a font name with different casing and see if dialog doesn't
    pop anymore

  • Try a font with different (localized) names like MS ゴシック and
    see if dialog doesn't pop anymore

  • Check Win7 with WPF target

Closes #9375

@ghost ghost added Area-Fonts Related to the font Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Apr 6, 2021
@github-actions

This comment has been minimized.

… variables to be nearby and not package related. add some doc comments.
@zadjii-msft
Copy link
Member

Thanks for making it a magic static - that's way better

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2021
@DHowett
Copy link
Member

DHowett commented Apr 7, 2021

"let me do my work," he says!

Windows 7 is now that work

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2021
…p if we can't use the quick-convenience methods to create the font set from the files on disk.
@miniksa
Copy link
Member Author

miniksa commented Apr 8, 2021

"let me do my work," he says!

Windows 7 is now that work

89e9870 (#9734) should cover it but I'll probably ask you to help triple check.

@skyline75489
Copy link
Collaborator

Maybe this isn't a good time...but this will conflict with #9201. I think we might want to merge #9201 first which will simplify this PR, too.

@DHowett
Copy link
Member

DHowett commented Apr 8, 2021

@skyline75489 I appreciate that, and thanks for your work in #9201! I'm inclined to merge this one first just so that we can be ready for the 1.7 release (next week; hit me up on Teams for why this is important :)) and then get reviews in 9201. I know that it complicates your life because you have some merging to do 😦 and I am sorry about that.

@DHowett DHowett changed the title Reduce instances of font fallback dialog through package font loading, basic name trimming, and revised fallback test Reduce instances of font fallback dialog Apr 8, 2021
@DHowett
Copy link
Member

DHowett commented Apr 8, 2021

I tested MS ゴシック using WPF; it did not trigger the fallback code path.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 8, 2021
@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Apr 8, 2021
@ghost
Copy link

ghost commented Apr 8, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that I didn't sign off.

@ghost ghost merged commit 7f5a19b into main Apr 8, 2021
@ghost ghost deleted the dev/miniksa/font_popup branch April 8, 2021 17:49
DHowett pushed a commit that referenced this pull request Apr 12, 2021
Reduce instances of font fallback dialog through package font loading,
basic name trimming, and revised fallback test

- Adjusts the font dialog to only show when we attempt last-chance
  resolution from our hardcoded list of font names with a flag instead
  of with a string comparison by name
- Adds a resolution step to trim the font name by word from the end and
  retry to attempt to resolve a proper font that just has a weight
  suffix
- Adds a second font collection to font loading that will attempt to
  locate all TTF files sitting next to our binary, like in our package

- [x] Wrote my font preference in the JSON as `Cascadia Code Heavy` and
  watched it quietly resolve to just `Cascadia Code` without the dialog.
- [x] Put a font that isn't registered with the system into the layout
  directory for the package, set it as my desired font in Terminal, and
  watched it load just fine.
- [x] Try a font name with different casing and see if dialog doesn't
  pop anymore
- [x] Try a font with different (localized) names like MS ゴシック and
  see if dialog doesn't pop anymore
- [x] Check Win7 with WPF target

Closes #9375

(cherry picked from commit 7f5a19b)
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Apr 13, 2021
@ghost
Copy link

ghost commented Apr 14, 2021

🎉Windows Terminal v1.7.1033.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Apr 14, 2021

🎉Windows Terminal Preview v1.8.1032.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Fonts Related to the font AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to find the selected font "Cascadia Mono".
4 participants